Fix HttpClientHandler.CreateNativeHandler implementation#116707
Fix HttpClientHandler.CreateNativeHandler implementation#116707jkoritzinsky merged 3 commits intodotnet:mainfrom
Conversation
jkoritzinsky
requested review from
a team,
AaronRobinsonMSFT,
Copilot,
jonathanpeppers and
rolfbjarne
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the implementation of HttpClientHandler.CreateNativeHandler to resolve #116698 by adjusting the native call signature and its invocation.
- Modified the native handler call by adding a null parameter to CallNative.
- Updated the extern method signature to include a parameter with an UnsafeAccessorType attribute, with conditional using directives for various target platforms.
Comments suppressed due to low confidence (2)
src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs:341
- Changing the CallNative invocation to pass a null argument may have platform-specific implications; please verify that the native implementation correctly handles a null parameter on all target platforms.
return (HttpMessageHandler)CallNative(null);
src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.AnyMobile.InvokeNativeHandler.cs:344
- The updated extern method signature now accepts a parameter annotated with UnsafeAccessorType. Ensure that this change aligns with the expected native binding on all platforms and that the attribute usage is properly resolved.
static extern GetHttpMessageHandlerReturnType CallNative([UnsafeAccessorType(GetHttpMessageHandlerType)] object? _);
|
should/can the message be more helpful than " Invalid usage of UnsafeAccessorAttribute." ? |
|
Tagging subscribers to this area: @dotnet/ncl |
|
looks like there are build failures |
| #if TARGET_ANDROID | ||
| using GetHttpMessageHandlerReturnType = object; |
There was a problem hiding this comment.
Why is Android different here? The handler is:
Should it also be HttpMessageHandler?
There was a problem hiding this comment.
The problem is we need to match the signature here:
https://github.com/dotnet/android/blob/04c44a87357285ae740ab0ed8976e8ff0645865a/src/Mono.Android/Android.Runtime/AndroidEnvironment.cs#L337
If that method returned HttpMessageHandler, then we wouldn't need this split.
There was a problem hiding this comment.
Returning object might be intentional, but I don't see a commit mentioning it. It could allow apps to trim away System.Net.Http.HttpMessageHandler if unused.
There was a problem hiding this comment.
It could allow apps to trim away System.Net.Http.HttpMessageHandler if unused.
I do not see why it would make a difference. The method below has HttpMessageHandler cast that will keep the type around.
| #if TARGET_ANDROID | ||
| using GetHttpMessageHandlerReturnType = object; | ||
| #elif TARGET_IOS || TARGET_MACCATALYST || TARGET_TVOS | ||
| using GetHttpMessageHandlerReturnType = System.Net.Http.HttpMessageHandler; |
There was a problem hiding this comment.
Should we unify the return type between the two to avoid this ifdef?
There was a problem hiding this comment.
@jonathanpeppers what do you think about us unifying the return type?
I'd like the simplicity of it.
There was a problem hiding this comment.
Trying it out here:
There is an app size regression test for Android "hello world", so I don't see an issue unifying -- assuming that passes.
Context: dotnet/runtime#116707 To prevent a `#if TARGET_ANDROID` in dotnet/runtime: #if TARGET_ANDROID using GetHttpMessageHandlerReturnType = object; #elif TARGET_IOS || TARGET_MACCATALYST || TARGET_TVOS using GetHttpMessageHandlerReturnType = System.Net.Http.HttpMessageHandler; #endif Let's unify and use `System.Net.Http.HttpMessageHandler` as the return type for `GetHttpMessageHandler()`.
Context: dotnet/runtime#116707 To prevent a `#if TARGET_ANDROID` in dotnet/runtime: #if TARGET_ANDROID using GetHttpMessageHandlerReturnType = object; #elif TARGET_IOS || TARGET_MACCATALYST || TARGET_TVOS using GetHttpMessageHandlerReturnType = System.Net.Http.HttpMessageHandler; #endif Let's unify and use `System.Net.Http.HttpMessageHandler` as the return type for `GetHttpMessageHandler()`. Removed a comment from Mono days, when we wouldn't want a cyclic reference between `Mono.Android.dll` and `System.Net.Http.dll`. This shouldn't be a problem anymore
|
/ba-g wasm failures unrelated |
|
/backport to release/10.0-preview6 |
|
Started backporting to release/10.0-preview6: https://github.com/dotnet/runtime/actions/runs/15717970523 |
7 participants
Fix #116698